Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPS: Parsers and Unit Tests #176

Open
wants to merge 206 commits into
base: main
Choose a base branch
from

Conversation

christopherkinyua
Copy link
Contributor

Related Issues

Closes #16

Summary

This PR is the first implementation step for integrating the GNSS Receiver with the system. This includes receiving the ASCII response from the receiver, parsing the response, and passing the parsed responses to their appropriate structs for BESTXYZA and TIMEA log function response.

Changes Made

The changes made to realize this were as follows:

  • Modified uart_handler to receive the GNSS response in interrupt mode.
  • Created a FreeRTOS thread task that will handle the receiving and parsing of the GNSS receiver response
  • Created files gps.c and gps_types.h containing the functions, data structs, and enum definitions.
  • Added the unit_test_gps.c file, which contains all the unit tests related to the GNSS receiver.
  • Modified LOG_message function to handle a larger buffer for printing to UART
  • Added a function GEN_int64_to_str that handles converting int64_t values into strings

Testing Instructions

I verified the accuracy of the GPS functions by running the run_all_unit_tests telecommand and ensured that all my unit tests passed. I have left my LOG_message function calls in to show the error that is being invoked. This will be removed during the review to declutter the unit test call function with all the prints to UART.

Special Notes for Your Reviewer(s)

There are some TODO tags within the code based on my current thoughts on the implementation I have done. Let me know if they are justified and we can discuss how to implement them and if there are other considerations I am missing.

During the Hexagon Office Tour, some of the engineers questioned my implementation method of using ASCII instead of binary. They were in favor of using binary mode. @robotoshi had a more in-depth conversation with the engineers than I did, he could inform us what he was talking about. Furthermore, they mentioned that they had an open-source library called EDIE for ASCII and binary decoding of the GNSS receiver.

The link is here: https://novatel.com/support/bulletins-alerts-release-notices/hexagon-novatel-announces-version-3-00-of-its-encode-decode-interface-engine

@christopherkinyua christopherkinyua added the requirement A mission requirement label Sep 14, 2024
@christopherkinyua christopherkinyua self-assigned this Sep 14, 2024
@christopherkinyua christopherkinyua linked an issue Sep 14, 2024 that may be closed by this pull request
@christopherkinyua christopherkinyua force-pushed the chris&kale/log-gps-position-to-filesystem-i16 branch from 906141c to ec23247 Compare September 14, 2024 19:45
@christopherkinyua christopherkinyua changed the title GPS: GNSS parser and unit tests GPS parser and unit tests Sep 14, 2024
Copy link
Contributor

@DeflateAwning DeflateAwning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really awesome!

A lot of convention changes.

One important, easy-to-implement change with the way we enable interrupts.

firmware/.stm32env Outdated Show resolved Hide resolved
firmware/Core/Inc/gps/gps.h Outdated Show resolved Hide resolved
firmware/Core/Inc/gps/gps_types.h Outdated Show resolved Hide resolved
firmware/Core/Inc/gps/gps_types.h Outdated Show resolved Hide resolved
firmware/Core/Src/gps/gps.c Outdated Show resolved Hide resolved
firmware/Core/Src/rtos_tasks/rtos_tasks.c Outdated Show resolved Hide resolved
firmware/Core/Src/rtos_tasks/rtos_tasks.c Outdated Show resolved Hide resolved
firmware/Core/Src/transforms/arrays.c Outdated Show resolved Hide resolved
firmware/Core/Src/transforms/arrays.c Outdated Show resolved Hide resolved
firmware/Core/Src/uart_handler/uart_handler.c Outdated Show resolved Hide resolved
@christopherkinyua
Copy link
Contributor Author

@DeflateAwning I've addressed all the PR comments except the one asking for some clarification. Let me know what you think and I can make some updates so you can continue with the PR review

@DeflateAwning
Copy link
Contributor

DeflateAwning commented Sep 20, 2024

Changes required:

  • Timeout in receiving from GPS (refer to Telecommand RTOS and/or EPS send_cmd_get_response function)
  • continue in RTOS task on error instead of returning
  • Add 1 telecommand called: TCMDEXEC_gps_set_enabled(0 or 1) -> For now, just call GPS_set_uart_interrupt_state(). Also, add a TODO to call the relevant EPS command to enable/disable power to the GPS.

TCMDEXEC_gps_set_enabled sequence

  1. Turn power on.
  2. Wait 1 second.
  3. Transmit the setup commands to the GPS (put it in the right mode, etc.) using HAL_UART_Transmit. These commands include "unlog all", etc.
  4. Wait 0.5 seconds.
  5. Call GPS_set_uart_interrupt_state()

After that, let's do some testing with this with the actual GPS! This looks really great!

@DeflateAwning
Copy link
Contributor

DeflateAwning commented Sep 20, 2024

Just merged the EPS firmware into main; you'll have to merge main into this branch, or rebase this branch to main. Sorry about that.

@DeflateAwning DeflateAwning removed the requirement A mission requirement label Oct 30, 2024
Copy link
Contributor

@DeflateAwning DeflateAwning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As requested, it looks good overall! There's a few things that should be fixed, and this branch should be rebased to main (or main merged into this branch), as it's starting to diverge a sizeable amount.

If there's a specific section you'd like me to check over, let me know! Obviously can't thoroughly review 2000 LOC several times.

firmware/Core/Src/gps/gps_internal_drivers.c Show resolved Hide resolved
firmware/Core/Src/rtos_tasks/rtos_tasks.c Outdated Show resolved Hide resolved
firmware/Core/Src/rtos_tasks/rtos_tasks.c Outdated Show resolved Hide resolved
@christopherkinyua
Copy link
Contributor Author

As requested, it looks good overall! There's a few things that should be fixed, and this branch should be rebased to main (or main merged into this branch), as it's starting to diverge a sizeable amount.

If there's a specific section you'd like me to check over, let me know! Obviously can't thoroughly review 2000 LOC several times.

Thanks for the quick look. I will address the comments you have mentioned.

I was hoping you would look into the gps_internal_drivers.c file. I have a function called GPS_send_cmd_get_response that essentially handles sending a command to the GPS and receiving the response. If you could give it a look and see if there is anything of note that stands out or doesn't inherently make sense, I will appreciate it.

@christopherkinyua christopherkinyua changed the title GPS parser and unit tests GPS: Send log command and receive log response Nov 29, 2024
@christopherkinyua
Copy link
Contributor Author

GPS Testing Successful. Modifying the PR to address the testing results. I will break up this PR and my code into segments for future integration.

@christopherkinyua christopherkinyua changed the title GPS: Send log command and receive log response GPS: Parsers and Unit Tests Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GPS: log GPS position to filesystem
2 participants